fix(mcp): bridge follow-ups from #109 review — smoke-script auth, MV3 heartbeat, pending-key collisions#110
Merged
Merged
Conversation
…MV3 heartbeat, pending-key collisions Addresses the review findings on #109 (all client-side; no behavior change to the tool contract). - scripts/test-mcp-bridge.mjs: send Authorization: Bearer $KEEPKEY_API_KEY. The merged vault (#360) bearer-authenticates POST /mcp, so the script as shipped could only ever exit 1 on a 401. Fails fast with guidance when the env var is unset, and names the cause on a 401. - mcpBridge.ts: 20s heartbeat ping. MV3 evicts an idle service worker after ~30s and an idle WebSocket does NOT prevent that — only WS traffic resets the timer. Without it the worker dies on a quiet machine, takes the socket with it, and nothing wakes it: agents would see bridge "down" indefinitely. The vault ignores frames matching no pending call, so no server change. - providerLog.ts: clip() can no longer throw. It runs on the SUCCESS path of every provider call, so a BigInt/circular stringify failure would have turned a good wallet response into a dApp-visible error. - providerLog.ts: key the approval queue by an internal uuid, not the dApp-supplied id. dApp ids are per-page counters, so two tabs collide on "1": one settle evicted the other tab's entry, and a Phase-2 bex_approve_request(id) would have resolved the WRONG origin's prompt. Fixed now, before the control tools land on top of it. - index.ts: the agent walletRequest path re-checks init after ensureStarted(), matching WALLET_REQUEST, so a vault-closed agent call gets the actionable "launch the Vault" error. Tests: 111 pass (providerLog 5 → 8; adds same-id two-tab isolation and the unserializable-payload regression). make type-check 15/15, make build green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…s-repo PR Writes up the vault-side finding from the #109 review so the vault owners can decide: /mcp cannot return its 401 because auth.requireAuth throws outside the try that maps HttpError → status, and Bun.serve has no top-level error handler, so the throw escapes fetch(). Includes the diff, the verification already run, the explicit caveat that the pre-fix symptom was never observed on a healthy vault (the original dropped-socket reading was taken while the vault was flapping), and a pointer to the existing branch/PR should they want it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-ups from the review of #109 (merged). All client-side; the tool contract is unchanged.
Fixes
1. Smoke script could never pass (must-fix).
scripts/test-mcp-bridge.mjssent noAuthorizationheader, but the merged vault (#360) runsauth.requireAuthonPOST /mcp— every run failed. It now sendsBearer $KEEPKEY_API_KEY, fails fast with guidance when unset, and names the cause on a 401. (Node's fetch sends noOrigin/Sec-Fetch-Site, which is what passes the vault's browser-exclusion check.)2. MV3 heartbeat. Chrome evicts an idle service worker after ~30s, and an idle WebSocket does not prevent that — only WS traffic resets the timer. Without a ping the worker dies on a quiet machine, takes the socket with it, and nothing wakes it back up: an agent polling
bex_statuswould seebridge: "down"indefinitely. A 20s ping keeps us inside the window; the vault ignores frames matching no pending call, so no server-side change.3.
clip()could throw on the success path. It runs on every provider call's result, so a BigInt/circularJSON.stringifyfailure would have turned a good wallet response into a dApp-visible error. Now returns'[unserializable]'.4. Approval queue keyed by dApp-supplied id. dApp ids are per-page counters, so two tabs collide on
1: one settle evicted the other tab's entry. Read-only today, but a Phase-2bex_approve_request(id)would have resolved the wrong origin's prompt — fixed now, before the control tools land on top of it. The queue is keyed by an internal uuid;idis retained for correlating with dApp logs.5. Agent path init parity.
walletRequestre-checks init afterensureStarted()likeWALLET_REQUESTdoes, so a vault-closed agent call gets the actionable "launch the Vault" error.Verification
make test111 pass (providerLog 5 → 8: adds same-id two-tab isolation + the unserializable-payload regression).make type-check15/15,make buildgreen.Live run against the vault, with a real pairing key — the fixed script doing what it could not do before:
Auth, the tool catalog, and
bex_status's truthful bridge-down reporting are all confirmed. The last mile still needs a human: flip "Enable Agent mode" in the extension options, reload the extension from this branch'sdist/, then re-run for exit 0 — that also exercises the heartbeat andbex_accounts.Related — a vault-side bug this surfaced
Sending the bearer surfaced a real vault bug:
/mcpcannot return its 401 at all.auth.requireAuth(rest-api.ts:1305) throws outside thetryat:1420that mapsHttpError→ status, andBun.servehas no top-levelerrorhandler, so the throw escapesfetch()and the caller gets a dropped socket instead of a 401.Independent of this PR (nothing here depends on it), but it's what makes a misconfigured key report
401rather than an inscrutablefetch failed. Written up for the vault owners inHANDOFF_vault_mcp_401.mdon this branch.🤖 Generated with Claude Code